feat: allow to httpClient use HTTP2 first #5332
Conversation
WalkthroughA new update introduces the option to use HTTP/2 in Egg.js's HTTP client by adding the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
lib/core/httpclient_next.js (1)
Line range hint
18-18: Use optional chaining for better safety.Enhance the safety and readability of the code by using optional chaining when accessing nested properties.
- options.tracer = options.ctx.tracer; + options.tracer = options.ctx?.tracer;config/config.default.js (1)
Line range hint
1-1: Remove redundant 'use strict' directive.Since ES6 modules are strict by default, the 'use strict' directive is unnecessary.
- 'use strict';test/lib/core/httpclient.test.js (1)
Line range hint
44-46: Avoid duplicate setup hooks.Combine the setup logic into a single
beforehook to improve the clarity and maintainability of the test suite.- before(() => { - client = new Httpclient({ - deprecate: () => {}, - config: { - httpclient: { - request: {}, - httpAgent: {}, - httpsAgent: {}, - }, - }, - }); - client.on('request', info => { - info.args.headers = info.args.headers || {}; - info.args.headers['mock-traceid'] = 'mock-traceid'; - info.args.headers['mock-rpcid'] = 'mock-rpcid'; - }); - - clientNext = new HttpclientNext({ - config: { - httpclient: { - request: {}, - }, - }, - }); - clientNext.on('request', info => { - info.args.headers = info.args.headers || {}; - info.args.headers['mock-traceid'] = 'mock-traceid'; - info.args.headers['mock-rpcid'] = 'mock-rpcid'; - }); - }); - before(async () => { - url = await utils.startLocalServer(); - }); + before(async () => { + client = new Httpclient({ + deprecate: () => {}, + config: { + httpclient: { + request: {}, + httpAgent: {}, + httpsAgent: {}, + }, + }, + }); + client.on('request', info => { + info.args.headers = info.args.headers || {}; + info.args.headers['mock-traceid'] = 'mock-traceid'; + info.args.headers['mock-rpcid'] = 'mock-rpcid'; + }); + + clientNext = new HttpclientNext({ + config: { + httpclient: { + request: {}, + }, + }, + }); + clientNext.on('request', info => { + info.args.headers = info.args.headers || {}; + info.args.headers['mock-traceid'] = 'mock-trageid'; + info.args.headers['mock-rpcid'] = 'mock-rpcid'; + }); + + url = await utils.startLocalServer(); + });[REFACTOR_SUGGESTion]
index.d.ts (1)
Line range hint
792-792: Type Definition Issue: Use ofvoidin Union TypeThe use of
voidin a union type can lead to confusion and potential runtime errors, as indicated by the static analysis tool.- sendTo(pid: number, action: string, data: any): void; + sendTo(pid: number, action: string, data: any): undefined;This change clarifies the expected behavior when the function does not return a value.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- config/config.default.js (2 hunks)
- index.d.ts (1 hunks)
- lib/core/httpclient_next.js (1 hunks)
- package.json (1 hunks)
- test/fixtures/apps/httpclient-http2/app.js (1 hunks)
- test/fixtures/apps/httpclient-http2/config/config.default.js (1 hunks)
- test/fixtures/apps/httpclient-http2/package.json (1 hunks)
- test/lib/core/httpclient.test.js (3 hunks)
Files skipped from review due to trivial changes (3)
- package.json
- test/fixtures/apps/httpclient-http2/config/config.default.js
- test/fixtures/apps/httpclient-http2/package.json
Additional context used
Biome
test/fixtures/apps/httpclient-http2/app.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
lib/core/httpclient_next.js
[error] 18-18: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
test/lib/core/httpclient.test.js
[error] 44-46: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
index.d.ts
[error] 792-792: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Additional comments not posted (5)
test/fixtures/apps/httpclient-http2/app.js (1)
16-18: Good use of method delegation incurl.The
curlmethod effectively delegates torequest, maintaining DRY principles and ensuring consistent behavior.lib/core/httpclient_next.js (2)
11-11: Proper handling ofallowH2configuration.The constructor correctly initializes the
allowH2property from the configuration, ensuring that HTTP/2 settings are configurable.
Line range hint
24-24: Effective use of method delegation incurl.The
curlmethod properly delegates torequest, ensuring consistency and reusability of the request handling logic.config/config.default.js (1)
307-307: Proper configuration addition for HTTP/2 preference.The addition of
allowH2to thehttpclientconfiguration is well-implemented, aligning with the PR's objective to prefer HTTP/2.index.d.ts (1)
299-299: Updated HttpClient Configuration PropertiesThe updates to
EggHttpClientConfigintroduce default values and new properties to support HTTP/2 configurations. This is in alignment with the PR objectives to enhance HTTP client capabilities. It's good practice to include default values in TypeScript interfaces for clarity and to prevent runtime errors.However, ensure that these default values are consistently handled in the implementation to avoid any unexpected behavior.
Also applies to: 301-301, 307-307, 310-310
| @@ -0,0 +1,21 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Remove redundant 'use strict' directive.
Since ES6 modules are strict by default, the 'use strict' directive is unnecessary.
- 'use strict';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'use strict'; |
Tools
Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
| class CustomHttpClient extends app.HttpClientNext { | ||
| request(url, opt) { | ||
| return new Promise(resolve => { | ||
| assert(/^http/.test(url), 'url should start with http, but got ' + url); |
There was a problem hiding this comment.
Improve URL validation regex.
The current regex ^http matches any string starting with "http" which could include invalid protocols like "httpx". Consider refining it to match only "http" or "https".
- assert(/^http/.test(url), 'url should start with http, but got ' + url);
+ assert(/^(http|https):/.test(url), 'url should start with http or https, but got ' + url);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert(/^http/.test(url), 'url should start with http, but got ' + url); | |
| assert(/^(http|https):/.test(url), 'url should start with http or https, but got ' + url); |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #5332 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 36 36
Lines 3628 3631 +3
Branches 520 520
=======================================
+ Hits 3623 3626 +3
Misses 5 5 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai 跟进此 pr 后续合并到 master 分支 |
|
Tip For best results, initiate chat on the files or code changes. @fengmk2 To follow up on this PR's progress towards merging into the master branch, you can track its status directly on GitHub. You may also consider setting up notifications for any updates to this PR to stay informed about changes, reviews, or merge events. If you need further assistance with setting up these notifications or have any other questions regarding the PR management process, feel free to ask! |
|
@coderabbitai 请创建一个 issue 跟进这个 pr 后续会被合并到 master 分支 |
|
Tip For best results, initiate chat on the files or code changes. @fengmk2 I've created an issue to track the progress of PR #5332 towards merging into the master branch. You can follow and manage the issue here: Issue #5333. If there's anything else you need, feel free to let me know! 🐰 Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Create a GitHub issue to track the PR progress towards merging into the master branch.
gh issue create --title "Track Progress of PR #5332 Towards Merging into Master" --body "This issue is to track the progress of PR #5332 (https://github.com/eggjs/egg/pull/5332) as it moves towards merging into the master branch. The PR aims to update the httpClient to prefer HTTP/2 based on changes from https://github.com/node-modules/urllib/pull/516." --assignee fengmk2
Length of output: 417 |
base on node-modules/urllib#516
Summary by CodeRabbit
New Features
allowH2property when using the next-generation HTTP client.Bug Fixes
Improvements
urllib-nextdependency to version^3.26.0for better performance and stability.